feat(brands): add explicit brand status-transition endpoint (LLMO-5587)#2621
Conversation
PR1 of the staged active->pending demotion fix. Adds an additive, intentful status-transition path so legitimate demotions have a sanctioned route before the generic PATCH is locked down in PR3. - PATCH /v2/orgs/:spaceCatId/brands/:brandId/status -> transitionBrandStatusForOrg - setBrandStatus storage fn (minimal: status + updated_by, no child-table sync) - 23514 chk_active_brand_has_site_id -> 400 mapping lifted from #2504 (Igor Grubic) - OpenAPI spec + capability + tests (5 storage, 10 controller) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a minor release when merged. |
aliciadriani
left a comment
There was a problem hiding this comment.
Review — LLMO-5587 brand status-transition endpoint (#2621)
Summary: Adds an additive, intentful PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status endpoint backed by a minimal setBrandStatus (status + updated_by only, no child sync). This is the sanctioned demotion path the #2637 guard routes callers to. Clean and purely additive; full suite green. One real gap inline.
Sequencing
This PR should merge first — #2637's 409 message references this endpoint as the demotion path. This PR is independently functional on its own.
Should Fix
deleted→active/pendingreactivation isn't guarded or tested (inline on brands-storage.js / controller).
What's Good
- Right design: a focused, intentful status setter kept separate from the guarded generic
updateBrand, so the demotion guard can't be bypassed accidentally. - 23514
chk_active_brand_has_site_id→ 400 mapping lifted from #2504 and credited to Igor. - Mirrors sibling brand handlers exactly (org/access/postgrest/resolve guards); OpenAPI + capability wired; good test coverage.
|
|
||
| const { data, error } = await postgrestClient | ||
| .from('brands') | ||
| .update({ status, updated_by: updatedBy }) |
There was a problem hiding this comment.
Should Fix: this update isn't scoped against soft-deleted brands. If resolveBrandUuid resolves a status='deleted' brand, this endpoint will silently reactivate it. Either intend that (document it) or block it — simplest is to add .neq('status', 'deleted') to the filter so a deleted brand matches no row and the controller returns 404. Add a test for the deleted-brand case.
| if (!hasText(brandId)) { | ||
| return badRequest('Brand ID required'); | ||
| } | ||
| if (status !== 'active' && status !== 'pending') { |
There was a problem hiding this comment.
Note: status is validated to {active, pending} but the brand's current status isn't checked — see the storage comment about not resurrecting deleted brands. Worth a test asserting a transition on a soft-deleted brand returns 404.
… brand (PR #2621 review) setBrandStatus now filters the update with .neq('status','deleted'), so a soft-deleted brand matches no row and the transition endpoint returns 404 instead of silently reactivating it. Adds storage + controller tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aliciadriani
left a comment
There was a problem hiding this comment.
Review fix applied — b55974a0
Addressed the Should-Fix (deleted-brand reactivation). Full suite green (11818 passing); ESLint clean.
setBrandStatusnow filters the update with.neq('status','deleted'), so a soft-deleted brand matches no row and the endpoint returns 404 rather than silently reactivating it.- Added tests: storage (
does not resurrect a soft-deleted brand → null) and controller (transitionBrandStatusForOrg returns 404 when the brand is soft-deleted).
Sequencing reminder: merge this PR before #2637 (its 409 message points here).
| // Do not resurrect a soft-deleted brand via a status transition — a deleted | ||
| // brand matches no row here, so the caller gets a 404 (use a dedicated | ||
| // undelete flow if reactivation is ever needed). | ||
| .neq('status', 'deleted') |
There was a problem hiding this comment.
✅ Fixed (b55974a0): .neq('status','deleted') excludes soft-deleted brands from the status update — they match no row, so setBrandStatus returns null and the controller responds 404 (no accidental undelete). Covered by new storage + controller tests.
…ForOrg (LLMO-5587) codecov flagged 2 uncovered diff lines (brands.js: the !hasText(spaceCatId) -> 'Organization ID required' branch). PR1 tested the invalid-UUID case but not a missing org id. Add that test. Test-only; no source change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - clean, focused additive endpoint that follows existing patterns exactly.
Complexity: HIGH - medium-to-large diff; new API surface (OpenAPI contract change).
Changes: Adds a dedicated PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status endpoint for intentful brand status transitions, with storage function, OpenAPI spec, and comprehensive tests (9 files).
Note: Recommend a human read before merge - this change modifies a shared API contract (docs/openapi/brands-v2-api.yaml). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (2): minor issues and suggestions
- nit: OpenAPI description says "the generic PATCH refuses that transition" but that guard ships in PR3, not this PR - consumers reading the spec today may be confused by behavior that does not yet exist -
docs/openapi/brands-v2-api.yaml:48 - suggestion: An explicit controller-level test for the
chk_active_brand_has_site_id400 path (activate brand without site) would document the end-to-end contract; currently only tested at the storage layer -test/controllers/brands.test.js
Spec-divergence: no - PR implements the staged rollout described in the body; no referenced spec to diverge from.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 11m 4s | Cost: $6.85 | Commit: d497ace55211d62fb4361808b8de438946b325dd
If this code review was useful, please react with 👍. Otherwise, react with 👎.
OpenAPI: soften forward-looking language — the generic PATCH demotion guard ships in PR3, not here; change "refuses" (present tense) to "will refuse" with an explicit (LLMO-5587 PR3) callout so consumers aren't confused by behaviour that doesn't exist yet. Test: add controller-level test for the chk_active_brand_has_site_id → 400 path (activate brand without base site). Documents the end-to-end contract from HTTP surface to DB constraint mapping, complementing the existing setBrandStatus storage-layer coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed both nits from the MysticatBot review in `0f520a3`: nit (OpenAPI forward-looking language): Changed "refuses that transition (LLMO-5587)" → "will refuse that transition" with an explicit "(LLMO-5587 PR3)" callout. Consumers reading the spec today see the description is forward-looking, not current behaviour. suggestion (controller-level test for |
…LMO-5587)
Istanbul flagged lines 71-72 and 157 as uncovered branches — all existing
tests pass real objects to getDeliveryConfig/getHlxConfig, so the || {}
right-hand side never fires.
- getPreflightMissingConfigLabels: one test with null delivery and helix
configs covers both fallbacks in a single call (lines 71-72).
- isPreflightSiteConfigReady: a stub returns a valid delivery config on
the first call (so getPreflightMissingConfigLabels sees no missing
labels) and null on the second call (line 157), exercising the fallback
without early-returning from the missing-labels guard.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ilities (LLMO-5587) facs-capabilities.js was introduced after this branch was cut; bring it in now and add the new status-transition route under llmo/can_configure, adjacent to the generic PATCH /v2/orgs/:spaceCatId/brands/:brandId entry. Fixes the facs-capabilities invariant test: every route in routes/index.js must appear in PRODUCTS_ROUTES or INTERNAL_ROUTES. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Keep the /status route entry added by this PR; main had no equivalent line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# [1.599.0](v1.598.1...v1.599.0) (2026-06-25) ### Features * **brands:** add explicit brand status-transition endpoint (LLMO-5587) ([#2621](#2621)) ([7fbebb6](7fbebb6)), closes [#2504](#2504) [#2593](#2593) [#2137](#2137) [#2139](#2139) [#2637](#2637) [#2139](#2139) [#2137](#2137) [#2139](#2139) [#2504](#2504) [#2593](#2593) [#2300](#2300)
|
🎉 This PR is included in version 1.599.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
…psertBrand (LLMO-5587) (#2637) ## Why Active brands can be silently demoted to `pending` by the per-brand write primitives (`updateBrand` / `upsertBrand`) — no error to the editor, no alert. A demoted brand stops binding in the scheduler (the resolver requires `status == active`) and its site goes dark. This took **express.adobe.com** offline on 2026-06-05 (brand `40f16625`); same class as the MongoDB and Merck/MSD incidents. Incident: [LLMO-5587](https://jira.corp.adobe.com/browse/LLMO-5587). ## Deploy gate This PR's 409 demotion guard must **NOT** deploy until elmo-ui #2139 is live in prod. Otherwise elmo's still-generic "Move to pending" hits the guard and 409s (loud, not silent). **Order:** spacecat #2621 → elmo #2137 → elmo #2139 (prod) → this PR (#2637). ## What The generic per-brand write paths now **refuse** to silently demote an active brand to pending; intentful demotions go through the dedicated status-transition endpoint added in #2621. - **`updateBrand`** — when a PATCH would take a persisted-`active` brand to `pending` → **409** `brand_status_demotion_not_allowed`. - **`upsertBrand`** — when a by-name (`org, name`) create/upsert would resolve onto an existing **active** brand and demote it → same **409**. (Closes the M1 collision vector.) - The single existing-row fetch is reused for all three brand guards (immutability, demotion, active-without-site). ## Credit — re-landed from #2504 (@igor-grubic) #2504 was closed in favor of this staged work; its complementary guards are re-landed here and credited in the code: - the **`23514` / `chk_active_brand_has_site_id` → 400** mapping (covers the SELECT→write race), and - the **active-without-site promotion guard** (`status:'active'` with no `site_id` → 400). - Its **existing-fetch pattern** is the basis for the fetch here, broadened from `site_id`-only to also read `status` for the demotion comparison. ## Status-code semantics - **409** — demotion of an active brand via the generic path (a routing/state conflict; the caller should use the `/status` endpoint). - **400** — row-invariant violations (activate-without-site, `23514`). ## Alerting — `BrandDemotionBlocked` metric A best-effort CloudWatch **EMF** metric is emitted from the reject branch (never affects the response), modeled on the LLMO-5150 P1 pattern: - **Namespace:** `Mysticat/Brands` · **Metric:** `BrandDemotionBlocked` (Count) - **Dimensions:** `Environment`, `Operation` (`updateBrand` | `createBrand`), `Product` (`x-product`) - Each rejection also logs a **WARN** naming the caller (`org`, `brand`, `updatedBy`) for attribution. - **Alarm suggestion:** P1 on `sum(BrandDemotionBlocked) > 0` per `Environment` (optionally split by `Operation`/`Product`); when it fires, query the WARN logs to identify which caller still demotes via the generic path. ## Sequencing **#2621 (the `/status` transition endpoint) should merge first** — the 409 message references that endpoint as the sanctioned path for intentful demotions. That said, **this PR is independently functional**: the guard and metric work standalone; callers migrate to the `/status` endpoint when the 409 + message tells them to (PR2 / elmo-ui coordination was intentionally dropped). ## Related - [LLMO-5587](https://jira.corp.adobe.com/browse/LLMO-5587) — incident - #2621 — PR1, explicit `/status` transition endpoint (the sanctioned demotion path) - #2504 — closed; re-landed from (@igor-grubic) ## Testing - Storage guard suite (demotion 409 ×2, promotion allowed, no-false-positive on status-absent edit, promote-without-site 400, `23514` ×2) - Controller 409 cases for `updateBrandForOrg` + `createBrandForOrg`; `metrics-emf` namespace test - Full suite green; ESLint clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Alicia Adriani <aadriani+adobe@adobe.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Alicia Adriani <aadriani@adobe.com>
# [1.600.0](v1.599.0...v1.600.0) (2026-06-25) ### Features * **brands:** guard against active->pending demotion in updateBrand/upsertBrand (LLMO-5587) ([#2637](#2637)) ([410ae58](410ae58)), closes [#2139](#2139) [#2621](#2621) [#2137](#2137) [#2139](#2139) [#2504](#2504) [#2621](#2621) [#2621](#2621) [#2504](#2504)
…2691) ##⚠️ Back office Outage mitigation — revert of #2686 Reverts the http-utils dependency bump from **#2686** (`96c5c104`), downgrading `@adobe/spacecat-shared-http-utils` **1.31.0 → 1.30.0**. **Why:** Back office production outage that began after the 1.31.0 bump deployed. 1.31.0 (PR adobe/spacecat-shared#1712) shipped — alongside `getFacsPermissions()` — the **IMS-handler RBAC block** (blocks IMS auth for RBAC-enabled orgs), an auth-path change the back office relies on. That is the most credible cause; detailed root cause to follow from @ravverma. **Scope:** only the http-utils version + its lockfile entry change (verified: single `version` line, no unrelated dep drift). The three feature PRs merged after #2686 (#2621, #2637, #2681) are **unaffected**. **Trade-off:** this re-breaks the `GET /user/capabilities` JWT-layer surfacing that #2686 fixed — but those endpoints are dev-gated (`AWS_ENV=dev`) and not prod-facing, so restoring back-office availability wins. **Re-land options:** - ship a patched http-utils that keeps `getFacsPermissions()` but gates the IMS-handler RBAC block behind a verified flag, then re-bump. - re-introduce the bump with the facsWrapper attachment once the back-office auth interaction is fixed + tested. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## [1.601.2](v1.601.1...v1.601.2) (2026-06-25) ### Bug Fixes * **deps:** redeploy http-utils 1.30.0 revert to prod (back office outage) ([#2692](#2692)) ([5c3cb11](5c3cb11)), closes [#2691](#2691) ### Reverts * fix(deps): bump spacecat-shared-http-utils 1.30.0 to 1.31.0 ([#2691](#2691)) ([d8c8d8b](d8c8d8b)), closes [#2686](#2686) [adobe/spacecat-shared#1712](adobe/spacecat-shared#1712) [#2686](#2686) [#2621](#2621) [#2637](#2637) [#2681](#2681)
Why
Active brands can be silently demoted to
pendingby the per-brand write primitives (updateBrand/upsertBrand) — no error to the editor, no alert. A demoted brand stops binding in the scheduler (the resolver requiresstatus == active) and its site goes dark. This took express.adobe.com offline on 2026-06-05 (brand40f16625, org5d4e5082); same class as the MongoDB and Merck/MSD incidents documented in #2593. Incident: LLMO-5587.What — PR1 of a staged rollout (additive)
Expand → migrate → contract. This is the additive expand step: a dedicated, intentful status-transition endpoint so legitimate demotions have a sanctioned path before the generic
PATCHis locked down.PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status→transitionBrandStatusForOrg(capabilityorganization:write)setBrandStatusstorage fn — minimal (status +updated_byonly, no child-table sync), deliberately separate fromupdateBrandso it carries no demotion guard and the generic path can.{ "status": "active" | "pending" };400invalid status / activate-without-site,403/404/503mirror the sibling brand handlers.active ⇒ site_id) stays at the data layer viachk_active_brand_has_site_id.Credit — lifted from #2504 (@igor-grubic)
The
23514/chk_active_brand_has_site_id→400mapping is lifted verbatim from @igor-grubic's #2504 (closed in favor of this staged work). PR3 will additionally adapt #2504'sneedsExistingFetchexisting-row fetch (broadened toselect('status')) for the demotion comparison — credit carried forward there too. Pattern parallel: #2593 (@byteclimber) anchors immutability in the samebrands-storage.jsblock.Staged rollout / dependency chain
moveToPending(andapprove) onto this endpoint. Must deploy before PR3.409active→pending guard on the genericPATCH /brands/:brandId(+upsertBrandby-name collision), re-land fix(brands): guard updateBrand against active-without-site_id (LLMO-5183) #2504's guards, add aBrandDemotionBlockedmetric. Must NOT deploy before elmo-ui chore: remove dead llmo-customer-config endpoint code #2139 is live in prod, or it breaks elmo-ui's still-genericmoveToPendingwith a loud 409.Full deploy order: spacecat #2621 → elmo #2137 → elmo #2139 (prod) → spacecat #2637.
Related
baseSiteIdimmutable (merged); same primitive / enforcement patternuq_brand_name_per_org(open)Testing
setBrandStatus: 5 unit tests ·transitionBrandStatusForOrg: 10 controller testsdocs:lint(OpenAPI) valid🤖 Generated with Claude Code